Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for the build_from_source npm config #99

Merged
merged 5 commits into from
Apr 21, 2019
Merged

Add support for the build_from_source npm config #99

merged 5 commits into from
Apr 21, 2019

Conversation

adipascu
Copy link
Contributor

@adipascu adipascu commented Apr 5, 2019

Implements #98

rc.js Outdated Show resolved Hide resolved
Copy link
Member

@vweevers vweevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests to rc-test.js?

@adipascu
Copy link
Contributor Author

adipascu commented Apr 5, 2019

I have updated the tests.

Seems like this line is no longer required for my build, removing this line also passes the existing tests.
Npm sets flags into process.env using this piece of code. Any flag like --some-str gets set into process.env as npm_config_some_str. This might also apply to the target, runtime and arch flags.

Might be a breaking change for some peaople, leaving it as is for now.

Edit: I am ending my day, feel free to edit or add new commits. 👋

@vweevers
Copy link
Member

vweevers commented Apr 5, 2019

Seems like this line is no longer required for my build, removing this line also passes the existing tests.

Yeah, I'm also a little fuzzy on the precedence here, I wanna take a closer look myself.

@vweevers
Copy link
Member

vweevers commented Apr 5, 2019

How should it work when you have build_from_source=true in npmrc, but install with npm i --build-from-source=non-matching-package-name? That should result in false, right?

@adipascu
Copy link
Contributor Author

adipascu commented Apr 5, 2019

The settings from .npmrc have priority over flags passed in the CLI.
If you pass both, npm will set the env value to whatever is inside .npmrc, if this is a package name, it will only rebuild that package. If its true it will rebuild all packages.

@vweevers
Copy link
Member

vweevers commented Apr 5, 2019

The settings from .npmrc have priority over flags passed in the CLI.

Generally they don't. And for build-from-source that isn't true either; in node-pre-gyp the dashed command line option takes precedence over the underscored rc/env option:

var source_build = gyp.opts['build-from-source'] || gyp.opts.build_from_source

So to stay in line with node-pre-gyp, we should also modify this to support --build-from-source=x:

prebuild-install/rc.js

Lines 12 to 26 in ba21172

var npmargs = ['prebuild', 'compile', 'build-from-source', 'debug']
try {
var npmArgv = JSON.parse(env.npm_config_argv).cooked
for (var i = 0; i < npmargs.length; ++i) {
if (npmArgv.indexOf('--' + npmargs[i]) !== -1) {
process.argv.push('--' + npmargs[i])
}
if (npmArgv.indexOf('--no-' + npmargs[i]) !== -1) {
process.argv.push('--no-' + npmargs[i])
}
}
if ((i = npmArgv.indexOf('--download')) !== -1) {
process.argv.push(npmArgv[i], npmArgv[i + 1])
}
} catch (e) { }

And move this logic accordingly:

prebuild-install/rc.js

Lines 32 to 33 in ba21172

var sourceBuild = env.npm_config_build_from_source
var buildFromSource = sourceBuild === pkg.name || (sourceBuild === true || sourceBuild === 'true')

But we can move those tasks to a follow-up PR, because it's about a separate feature. Before that, perhaps we can change (some of) the rc tests to functional tests - i.e. running npm install on a mock package.

rc.js Outdated Show resolved Hide resolved
Co-Authored-By: adipascu <1521321+adipascu@users.noreply.github.com>
@adipascu
Copy link
Contributor Author

@vweevers how can I help you merge this?

@vweevers
Copy link
Member

I need some more eyes on this.

@vweevers vweevers merged commit 79f67f7 into prebuild:master Apr 21, 2019
@vweevers
Copy link
Member

5.3.0

@adipascu
Copy link
Contributor Author

Thanks guys, nice working with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants